Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bugfix: In legacy mode, call suspended tree's unmount effects when it is deleted #24400

Merged
merged 2 commits into from
Apr 19, 2022

Conversation

acdlite
Copy link
Collaborator

@acdlite acdlite commented Apr 19, 2022

When a suspended tree switches to a fallback, we unmount the effects. If the suspended tree is then deleted, there's a guard to prevent us from unmounting the effects again (added in ec52a56)

However, in legacy mode, we don't unmount effects when a tree suspends. So if the suspended tree is then deleted, we do need to unmount the effects.

This only affects apps using ReactDOM.render, which is deprecated (not supported) in React 18+. (And React Native, which hasn't finished adopting the React 18 behavior yet.)

Until ReactDOM.render is completely removed, though, we need to make sure to add concurrent mode checks whenever we add Offscreen-related logic to a shared codepath.

@facebook-github-bot facebook-github-bot added CLA Signed React Core Team Opened by a member of the React Core Team labels Apr 19, 2022
@acdlite acdlite changed the title Fix missing unmount legacy Bugfix: Missing unmount when suspended tree deleted Apr 19, 2022
@acdlite acdlite changed the title Bugfix: Missing unmount when suspended tree deleted Bugfix: In legacy mode, call suspended tree's unmount effects when it is deleted Apr 19, 2022
@sizebot
Copy link

sizebot commented Apr 19, 2022

Comparing: 2bf5eba...ca98b30

Critical size changes

Includes critical production bundles, as well as any change greater than 2%:

Name +/- Base Current +/- gzip Base gzip Current gzip
oss-stable/react-dom/cjs/react-dom.production.min.js +0.03% 131.43 kB 131.47 kB +0.03% 42.05 kB 42.07 kB
oss-experimental/react-dom/cjs/react-dom.production.min.js +0.03% 136.70 kB 136.74 kB +0.03% 43.64 kB 43.65 kB
facebook-www/ReactDOM-prod.classic.js +0.08% 435.12 kB 435.45 kB +0.04% 79.88 kB 79.91 kB
facebook-www/ReactDOM-prod.modern.js +0.08% 420.12 kB 420.45 kB +0.04% 77.51 kB 77.54 kB
facebook-www/ReactDOMForked-prod.classic.js +0.08% 435.12 kB 435.45 kB +0.04% 79.88 kB 79.91 kB

Significant size changes

Includes any change greater than 0.2%:

Expand to show
Name +/- Base Current +/- gzip Base gzip Current gzip
facebook-www/ReactTestRenderer-dev.modern.js = 682.84 kB 681.44 kB = 147.30 kB 146.99 kB
facebook-www/ReactTestRenderer-dev.classic.js = 682.83 kB 681.43 kB = 147.30 kB 146.99 kB
facebook-react-native/react-test-renderer/cjs/ReactTestRenderer-dev.js = 661.48 kB 660.08 kB = 143.36 kB 143.05 kB
oss-experimental/react-test-renderer/umd/react-test-renderer.development.js = 679.67 kB 678.21 kB = 143.47 kB 143.14 kB
oss-experimental/react-test-renderer/cjs/react-test-renderer.development.js = 648.41 kB 646.99 kB = 141.92 kB 141.56 kB
oss-stable-semver/react-test-renderer/umd/react-test-renderer.development.js = 652.81 kB 651.36 kB = 138.15 kB 137.84 kB
oss-stable/react-test-renderer/umd/react-test-renderer.development.js = 652.81 kB 651.36 kB = 138.15 kB 137.84 kB
oss-stable-semver/react-test-renderer/cjs/react-test-renderer.development.js = 622.86 kB 621.44 kB = 136.67 kB 136.32 kB
oss-stable/react-test-renderer/cjs/react-test-renderer.development.js = 622.86 kB 621.44 kB = 136.67 kB 136.32 kB
react-native/implementations/ReactFabric-profiling.fb.js = 319.01 kB 318.28 kB = 57.16 kB 57.04 kB
react-native/implementations/ReactFabric-prod.fb.js = 292.12 kB 291.40 kB = 52.92 kB 52.82 kB
react-native/implementations/ReactFabric-profiling.js = 298.29 kB 297.56 kB = 53.70 kB 53.58 kB
react-native/implementations/ReactFabric-prod.js = 279.37 kB 278.65 kB = 50.54 kB 50.43 kB
facebook-react-native/react-test-renderer/cjs/ReactTestRenderer-profiling.js = 264.77 kB 264.09 kB = 47.92 kB 47.82 kB
facebook-react-native/react-test-renderer/cjs/ReactTestRenderer-prod.js = 249.98 kB 249.29 kB = 45.60 kB 45.49 kB

Generated by 🚫 dangerJS against ca98b30

Copy link
Member

@rickhanlonii rickhanlonii left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice find

When a suspended tree switches to a fallback, we unmount the effects.
If the suspended tree is then deleted, there's a guard to prevent us
from unmounting the effects again.

However, in legacy mode, we don't unmount effects when a tree suspends.
So if the suspended tree is then deleted, we do need to unmount
the effects.

We're missing a check for legacy/concurrent mode.
@acdlite acdlite merged commit ab9cdd3 into facebook:main Apr 19, 2022
facebook-github-bot pushed a commit to facebook/react-native that referenced this pull request Apr 26, 2022
Summary:
This sync includes the following changes:
- **[bd4784c8f](facebook/react@bd4784c8f )**: Revert #24236 (Don't recreate the same fallback on the client if hydrating suspends) ([#24434](facebook/react#24434)) //<dan>//
- **[6d3b6d0f4](facebook/react@6d3b6d0f4 )**: forwardRef et al shouldn't affect if props reused ([#24421](facebook/react#24421)) //<Andrew Clark>//
- **[bd0813766](facebook/react@bd0813766 )**: Fix: useDeferredValue should reuse previous value ([#24413](facebook/react#24413)) //<Andrew Clark>//
- **[9ae80d6a2](facebook/react@9ae80d6a2 )**: Suppress hydration warnings when a preceding sibling suspends ([#24404](facebook/react#24404)) //<Josh Story>//
- **[0dc4e6663](facebook/react@0dc4e6663 )**: Land enableClientRenderFallbackOnHydrationMismatch ([#24410](facebook/react#24410)) //<Andrew Clark>//
- **[354772952](facebook/react@354772952 )**: Land enableSelectiveHydration flag ([#24406](facebook/react#24406)) //<Andrew Clark>//
- **[392808a1f](facebook/react@392808a1f )**: Land enableClientRenderFallbackOnTextMismatch flag ([#24405](facebook/react#24405)) //<Andrew Clark>//
- **[1e748b452](facebook/react@1e748b452 )**: Land enableLazyElements flag ([#24407](facebook/react#24407)) //<Andrew Clark>//
- **[4175f0593](facebook/react@4175f0593 )**: Temporarily feature flag numeric fallback for symbols ([#24401](facebook/react#24401)) //<Ricky>//
- **[a6d53f346](facebook/react@a6d53f346 )**: Revert "Clean up Selective Hydration / Event Replay flag ([#24156](facebook/react#24156))" ([#24402](facebook/react#24402)) //<Ricky>//
- **[ab9cdd34f](facebook/react@ab9cdd34f )**: Bugfix: In legacy mode, call suspended tree's unmount effects when it is deleted ([#24400](facebook/react#24400)) //<Andrew Clark>//
- **[168da8d55](facebook/react@168da8d55 )**: Fix typo that happened during rebasing //<Andrew Clark>//
- **[8bc527a4c](facebook/react@8bc527a4c )**: Bugfix: Fix race condition between interleaved and non-interleaved updates ([#24353](facebook/react#24353)) //<Andrew Clark>//
- **[f7cf077cc](facebook/react@f7cf077cc )**: [Transition Tracing] Add Offscreen Queue ([#24341](facebook/react#24341)) //<Luna Ruan>//
- **[4fc394bbe](facebook/react@4fc394bbe )**: Fix suspense fallback throttling ([#24253](facebook/react#24253)) //<sunderls>//
- **[80170a068](facebook/react@80170a068 )**: Match bundle.name and match upper case entry points ([#24346](facebook/react#24346)) //<Sebastian Markbåge>//
- **[fea6f8da6](facebook/react@fea6f8da6 )**: [Transition Tracing] Add transition to OffscreenState and pendingSuspenseBoundaries to RootState ([#24340](facebook/react#24340)) //<Luna Ruan>//
- **[8e2f9b086](facebook/react@8e2f9b086 )**: move passive flag ([#24339](facebook/react#24339)) //<Luna Ruan>//
- **[55a21ef7e](facebook/react@55a21ef7e )**: fix pushTransition for transition tracing ([#24338](facebook/react#24338)) //<Luna Ruan>//
- **[069d23bb7](facebook/react@069d23bb7 )**:  [eslint-plugin-exhaustive-deps] Fix exhaustive deps check for unstable vars ([#24343](facebook/react#24343)) //<Afzal Sayed>//
- **[4997515b9](facebook/react@4997515b9 )**: Point useSubscription to useSyncExternalStore shim ([#24289](facebook/react#24289)) //<dan>//
- **[01e2bff1d](facebook/react@01e2bff1d )**: Remove unnecessary check ([#24332](facebook/react#24332)) //<zhoulixiang>//
- **[d9a0f9e20](facebook/react@d9a0f9e20 )**: Delete create-subscription folder ([#24288](facebook/react#24288)) //<dan>//
- **[f993ffc51](facebook/react@f993ffc51 )**: Fix infinite update loop that happens when an unmemoized value is passed to useDeferredValue ([#24247](facebook/react#24247)) //<Andrew Clark>//
- **[fa5800226](facebook/react@fa5800226 )**: [Fizz] Pipeable Stream Perf ([#24291](facebook/react#24291)) //<Josh Story>//
- **[0568c0f8c](facebook/react@0568c0f8c )**: Replace zero with NoLanes for consistency in FiberLane ([#24327](facebook/react#24327)) //<Leo>//
- **[e0160d50c](facebook/react@e0160d50c )**: add transition tracing transitions stack ([#24321](facebook/react#24321)) //<Luna Ruan>//
- **[b0f13e5d3](facebook/react@b0f13e5d3 )**: add pendingPassiveTransitions ([#24320](facebook/react#24320)) //<Luna Ruan>//

Changelog:
[General][Changed] - React Native sync for revisions 60e63b9...bd4784c

jest_e2e[run_all_tests]

Reviewed By: kacieb

Differential Revision: D35899012

fbshipit-source-id: 86a885e336fca9f0efa80cd2b8ca040f2cb53853
acdlite added a commit to acdlite/react that referenced this pull request Jun 2, 2022
Interleaves updates (updates that are scheduled while another render
is already is progress) go into a special queue that isn't applied until
the end of the current render. They are transferred to the "real" queue
at the beginning of the next render.

Currently we check during `setState` whether an update should go
directly onto the real queue or onto the special interleaved queue. The
logic is subtle and it can lead to bugs if you mess it up, as in facebook#24400.

Instead, this changes it to always go onto the interleaved queue. The
behavior is the same but the logic is simpler.

As a further step, we can also wait to update the `childLanes` until
the end of the current render. I'll do this in the next step.
acdlite added a commit to acdlite/react that referenced this pull request Jun 3, 2022
Interleaves updates (updates that are scheduled while another render
is already is progress) go into a special queue that isn't applied until
the end of the current render. They are transferred to the "real" queue
at the beginning of the next render.

Currently we check during `setState` whether an update should go
directly onto the real queue or onto the special interleaved queue. The
logic is subtle and it can lead to bugs if you mess it up, as in facebook#24400.

Instead, this changes it to always go onto the interleaved queue. The
behavior is the same but the logic is simpler.

As a further step, we can also wait to update the `childLanes` until
the end of the current render. I'll do this in the next step.
acdlite added a commit to acdlite/react that referenced this pull request Jun 3, 2022
Interleaves updates (updates that are scheduled while another render
is already is progress) go into a special queue that isn't applied until
the end of the current render. They are transferred to the "real" queue
at the beginning of the next render.

Currently we check during `setState` whether an update should go
directly onto the real queue or onto the special interleaved queue. The
logic is subtle and it can lead to bugs if you mess it up, as in facebook#24400.

Instead, this changes it to always go onto the interleaved queue. The
behavior is the same but the logic is simpler.

As a further step, we can also wait to update the `childLanes` until
the end of the current render. I'll do this in the next step.
acdlite added a commit to acdlite/react that referenced this pull request Jun 3, 2022
Interleaves updates (updates that are scheduled while another render
is already is progress) go into a special queue that isn't applied until
the end of the current render. They are transferred to the "real" queue
at the beginning of the next render.

Currently we check during `setState` whether an update should go
directly onto the real queue or onto the special interleaved queue. The
logic is subtle and it can lead to bugs if you mess it up, as in facebook#24400.

Instead, this changes it to always go onto the interleaved queue. The
behavior is the same but the logic is simpler.

As a further step, we can also wait to update the `childLanes` until
the end of the current render. I'll do this in the next step.
acdlite added a commit to acdlite/react that referenced this pull request Jun 6, 2022
Interleaves updates (updates that are scheduled while another render
is already is progress) go into a special queue that isn't applied until
the end of the current render. They are transferred to the "real" queue
at the beginning of the next render.

Currently we check during `setState` whether an update should go
directly onto the real queue or onto the special interleaved queue. The
logic is subtle and it can lead to bugs if you mess it up, as in facebook#24400.

Instead, this changes it to always go onto the interleaved queue. The
behavior is the same but the logic is simpler.

As a further step, we can also wait to update the `childLanes` until
the end of the current render. I'll do this in the next step.
acdlite added a commit that referenced this pull request Jun 6, 2022
* Always push updates to interleaved queue first

Interleaves updates (updates that are scheduled while another render
is already is progress) go into a special queue that isn't applied until
the end of the current render. They are transferred to the "real" queue
at the beginning of the next render.

Currently we check during `setState` whether an update should go
directly onto the real queue or onto the special interleaved queue. The
logic is subtle and it can lead to bugs if you mess it up, as in #24400.

Instead, this changes it to always go onto the interleaved queue. The
behavior is the same but the logic is simpler.

As a further step, we can also wait to update the `childLanes` until
the end of the current render. I'll do this in the next step.

* Move setState return path traversal to own call

A lot of the logic around scheduling an update needs access to the
fiber root. To obtain this reference, we must walk up the fiber return
path. We also do this to update `childLanes` on all the parent
nodes, so we can use the same traversal for both purposes.

The traversal currently happens inside `scheduleUpdateOnFiber`, but
sometimes we need to access it beyond that function, too.

So I've hoisted the traversal out of `scheduleUpdateOnFiber` into its
own function call that happens at the beginning of the
`setState` algorithm.

* Rename ReactInterleavedUpdates -> ReactFiberConcurrentUpdates

The scope of this module is expanding so I've renamed accordingly. No
behavioral changes.

* Enqueue and update childLanes in same function

During a setState, the childLanes are updated immediately, even if a
render is already in progress. This can lead to subtle concurrency bugs,
so the plan is to wait until the in-progress render has finished before
updating the childLanes, to prevent subtle concurrency bugs.

As a step toward that change, when scheduling an update, we should not
update the childLanes directly, but instead defer to the
ReactConcurrentUpdates module to do it at the appropriate time.

This makes markUpdateLaneFromFiberToRoot a private function that is
only called from the ReactConcurrentUpdates module.

* [FORKED] Don't update childLanes until after current render

(This is the riskiest commit in the stack. Only affects the "new"
reconciler fork.)

Updates that occur in a concurrent event while a render is already in
progress can't be processed during that render. This is tricky to get
right. Previously we solved this by adding concurrent updates to a
special `interleaved` queue, then transferring the `interleaved` queue
to the `pending` queue after the render phase had completed.

However, we would still mutate the `childLanes` along the parent path
immediately, which can lead to its own subtle data races.

Instead, we can queue the entire operation until after the render phase
has completed. This replaces the need for an `interleaved` field on
every fiber/hook queue.

The main motivation for this change, aside from simplifying the logic a
bit, is so we can read information about the current fiber while we're
walking up its return path, like whether it's inside a hidden tree.
(I haven't done anything like that in this commit, though.)

* Add 17691ac to forked revisions
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed React Core Team Opened by a member of the React Core Team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants